Skip to content

Conversation

@abelarismendy
Copy link
Contributor

@abelarismendy abelarismendy commented Dec 2, 2024

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a new order management system with local and remote data sources.
    • Added functionality to retrieve and update orders with improved error handling.
    • New screens for viewing order details and listing orders with navigation capabilities.
  • Improvements

    • Enhanced date formatting for better readability.
    • Streamlined order item display and interaction.
  • Bug Fixes

    • Improved error handling in order retrieval and updates.
  • Documentation

    • Updated internal documentation to reflect new order management features.

@coderabbitai
Copy link

coderabbitai bot commented Dec 2, 2024

📝 Walkthrough

Walkthrough

The pull request introduces significant updates to the order management system within the application. Key changes include the addition of local and remote data sources for orders, an order repository, and the corresponding models and use cases. The OrderBloc has been enhanced for better error handling and dependency injection. Additionally, the UI components for displaying order details and lists have been updated to reflect these changes, including navigation to order details. The DateUtils class has also been modified to improve date formatting methods.

Changes

File Path Change Summary
lib/core/utils/date_utils.dart - Updated formatDayMonth method to change date format from "MM/dd" to "d MMM".
- Added formatDateTime method to format DateTime objects as "d MMM y, HH:mm".
lib/features/orders/data/datasources/order_local_data_source.dart - Added OrderLocalDataSource interface with methods for caching, retrieving, clearing orders, and updating order status.
- Implemented OrderLocalDataSourceImpl class to manage SQLite database operations for orders.
lib/features/orders/data/datasources/order_remote_data_source.dart - Added OrderRemoteDataSource interface with methods for getting orders, updating order status, and creating orders.
- Implemented OrderRemoteDataSourceImpl class utilizing Firebase Firestore for data operations.
lib/features/orders/data/models/order_model.dart - Added OrderModel and OrderItemModel classes with constructors and methods for serialization and deserialization from maps.
lib/features/orders/data/repositories/order_repository_impl.dart - Implemented OrderRepositoryImpl class to manage orders through local and remote data sources, handling network connectivity and errors.
lib/features/orders/domain/entities/order.dart - Added Order and OrderItem classes, along with OrderStatus enum for order management.
lib/features/orders/domain/repositories/order_repository.dart - Introduced OrderRepository abstract class with methods for getting orders and updating order status.
lib/features/orders/domain/usecases/get_orders.dart - Added GetOrders class for retrieving orders from the repository.
lib/features/orders/domain/usecases/update_order_status.dart - Added UpdateOrderStatus use case for updating an order's status, along with UpdateOrderStatusParams class for parameters.
lib/features/orders/presentation/bloc/order_bloc.dart - Updated OrderBloc to use dependency injection for use cases and improved error handling in loading and updating orders.
lib/features/orders/presentation/bloc/order_event.dart - Updated OrderEvent class with constant constructors and added UpdateOrder event class for updating order status.
lib/features/orders/presentation/bloc/order_state.dart - Added OrdersError state class for error handling and updated OrdersLoaded to use a constant constructor.
lib/features/orders/presentation/screens/order_details_screen.dart - Introduced OrderDetailsScreen class for displaying detailed order information.
lib/features/orders/presentation/screens/order_list_screen.dart - Updated OrderListScreen to use entities instead of models and added navigation to OrderDetailsScreen.
lib/features/orders/presentation/widgets/order_item.dart - Modified OrderItem widget to simplify state management and enhance interactivity with an onTap callback.
lib/injection_container.dart - Updated service locator setup to include order-related functionalities, registering data sources, repositories, and use cases.

Suggested labels

enhancement

Suggested reviewers

  • RaulRinconX

🐰 In the meadow, orders hop,
With data sources, they won't stop.
From local to remote, they flow,
In screens and blocs, they brightly glow.
A date's new format, oh what a treat,
In our app, everything's now neat! 🌼


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@abelarismendy
Copy link
Contributor Author

closes #71

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 19

🧹 Outside diff range and nitpick comments (18)
lib/features/orders/presentation/bloc/order_bloc.dart (3)

36-36: Provide user-friendly error messages in OrdersError state

When emitting the OrdersError state, consider providing a user-friendly error message instead of the raw failure message.

Modify the emitted error message to be more user-friendly.


63-63: Provide user-friendly error messages in OrdersError state

When emitting the OrdersError state after a failed order update, consider providing a user-friendly error message.

Modify the emitted error message to be more informative for the user.


67-67: Ensure state consistency after order update

After successfully updating an order, the bloc adds a LoadOrders event. This could cause unnecessary reloading. Consider updating the local state directly to reflect the change without reloading all orders.

Refactor the state management to update the specific order in the current state.

lib/features/orders/data/repositories/order_repository_impl.dart (1)

66-66: Avoid using null in Right for void return types

Returning Right(null) may be confusing. Instead, use Right(unit) from the dartz package to represent a successful void result.

Apply this diff:

- return const Right<Failure, void>(null);
+ return Right<Failure, void>(unit);
lib/features/orders/presentation/widgets/order_item.dart (3)

58-64: Ensure color contrast for accessibility

The color used in the status label background and text should have sufficient contrast to be readable by users with visual impairments.

Consider reviewing the color choices to ensure accessibility compliance.


85-97: Consider default case handling in _getStatusColor

While all OrderStatus enum values are currently handled, adding a default case can prevent potential issues if new statuses are added in the future.

Apply this diff to include a default case:

      switch (status) {
        // existing cases
+     default:
+       return Colors.grey;
      }

41-45: Truncate long business names to prevent overflow

Ensure that very long business names do not cause layout issues by adding ellipses or limiting the maximum lines.

The current code uses maxLines: 1 and overflow: TextOverflow.ellipsis, which is good.

lib/features/orders/data/datasources/order_local_data_source.dart (1)

63-64: Consider wrapping deletions in transactions for data integrity

While you're already using a transaction, ensure that deletions are properly handled to prevent partial data states.

Review the transaction handling to ensure atomicity.

lib/features/orders/domain/repositories/order_repository.dart (1)

6-12: Consider adding documentation comments.

Adding documentation comments (///) for the abstract class and its methods would improve code maintainability and IDE support.

+/// Repository interface for managing orders.
 abstract class OrderRepository {
+  /// Retrieves a list of orders.
+  /// Returns a [Future] that completes with either a [Failure] or a [List] of [Order].
   Future<Either<Failure, List<entities.Order>>> getOrders();
 
+  /// Updates the status of an order.
+  /// 
+  /// Parameters:
+  /// - [orderId]: The unique identifier of the order
+  /// - [newStatus]: The new status to be applied
+  /// 
+  /// Returns a [Future] that completes with either a [Failure] or void.
   Future<Either<Failure, void>> updateOrderStatus(
     String orderId,
     entities.OrderStatus newStatus,
   );
 }
lib/features/orders/domain/usecases/get_orders.dart (1)

7-15: Consider using NoParams for consistency with clean architecture.

The implementation looks good, but for consistency with clean architecture patterns, consider using a NoParams class for the call method.

 class GetOrders {
   const GetOrders(this.repository);
 
   final OrderRepository repository;
 
-  Future<Either<Failure, List<order_entity.Order>>> call() async {
+  Future<Either<Failure, List<order_entity.Order>>> call(NoParams params) async {
     return repository.getOrders();
   }
 }
+
+/// Represents no parameters for use cases that don't require input
+class NoParams extends Equatable {
+  @override
+  List<Object?> get props => [];
+}
lib/core/utils/date_utils.dart (1)

20-21: Consider adding documentation and examples.

The new method looks good, but would benefit from documentation similar to other methods in the class.

+  /// Formats the given date and time into a string in the format "d MMM y, HH:mm".
+  /// Example output: "27 Apr 2023, 14:30"
   static String formatDateTime(DateTime dateTime) {
     return DateFormat('d MMM y, HH:mm').format(dateTime);
   }
lib/features/orders/presentation/bloc/order_state.dart (1)

24-31: Consider adding @immutable annotation.

The OrdersError state implementation looks good, but could benefit from explicit immutability annotation.

+import 'package:meta/meta.dart';

+@immutable
 class OrdersError extends OrderState {
   const OrdersError(this.message);
lib/features/orders/domain/usecases/update_order_status.dart (1)

17-28: Consider adding toString override for debugging.

The params class implementation looks good but could benefit from a toString override for better debugging experience.

   @override
   List<Object?> get props => <Object?>[orderId, newStatus];
+
+  @override
+  String toString() => 'UpdateOrderStatusParams(orderId: $orderId, newStatus: $newStatus)';
lib/features/orders/domain/entities/order.dart (1)

3-24: Add documentation and serialization methods to OrderStatus enum

Consider adding:

  1. Documentation comments explaining each status and valid state transitions
  2. A fromString factory constructor for safe deserialization
/// Represents the possible states of an order in the system.
enum OrderStatus {
  /// Initial state when order is created
  pending,
  /// Order has been confirmed by business
  confirmed,
  /// Order is ready for pickup/delivery
  ready,
  /// Order has been delivered/picked up
  completed,
  /// Order has been cancelled
  cancelled;

  /// Creates an OrderStatus from a string representation
  static OrderStatus fromString(String status) {
    return OrderStatus.values.firstWhere(
      (s) => s.name == status,
      orElse: () => throw ArgumentError('Invalid status: $status'),
    );
  }

  String get displayName => name[0].toUpperCase() + name.substring(1);
}
lib/features/orders/presentation/screens/order_list_screen.dart (1)

26-26: Consider adding pull-to-refresh functionality

The LoadOrders event is only dispatched once when the screen builds. Consider adding pull-to-refresh for better UX.

RefreshIndicator(
  onRefresh: () async {
    context.read<OrderBloc>().add(LoadOrders());
  },
  child: ListView.builder(...),
)
lib/features/orders/data/models/order_model.dart (1)

56-81: Add copyWith method to OrderItemModel

For better immutability support, consider adding a copyWith method.

OrderItemModel copyWith({
  String? id,
  String? name,
  int? quantity,
  double? price,
}) {
  return OrderItemModel(
    id: id ?? this.id,
    name: name ?? this.name,
    quantity: quantity ?? this.quantity,
    price: price ?? this.price,
  );
}
lib/features/orders/presentation/screens/order_details_screen.dart (2)

35-51: Consider improving status indicator accessibility.

The status indicator's color opacity of 0.1 might not provide sufficient contrast for all users. Consider:

  1. Increasing the background opacity
  2. Adding a semantic label for screen readers
 Container(
   padding: const EdgeInsets.symmetric(
     horizontal: 12,
     vertical: 6,
   ),
   decoration: BoxDecoration(
-    color: _getStatusColor(order.status).withOpacity(0.1),
+    color: _getStatusColor(order.status).withOpacity(0.15),
     borderRadius: BorderRadius.circular(16),
   ),
   child: Text(
     order.status.displayName,
+    semanticsLabel: 'Order status: ${order.status.displayName}',
     style: theme.textTheme.titleMedium?.copyWith(
       color: _getStatusColor(order.status),
       fontWeight: FontWeight.w500,
     ),
   ),
 ),

112-125: Consider future-proofing status colors and theme integration.

The current implementation could be improved by:

  1. Adding a default case for future status values
  2. Using theme colors instead of hardcoded ones
 Color _getStatusColor(order_entity.OrderStatus status) {
   switch (status) {
     case order_entity.OrderStatus.pending:
-      return Colors.orange;
+      return Theme.of(context).colorScheme.warning ?? Colors.orange;
     case order_entity.OrderStatus.confirmed:
-      return Colors.blue;
+      return Theme.of(context).colorScheme.primary;
     case order_entity.OrderStatus.ready:
-      return Colors.green;
+      return Theme.of(context).colorScheme.success ?? Colors.green;
     case order_entity.OrderStatus.completed:
-      return Colors.purple;
+      return Theme.of(context).colorScheme.tertiary ?? Colors.purple;
     case order_entity.OrderStatus.cancelled:
-      return Colors.red;
+      return Theme.of(context).colorScheme.error;
+    default:
+      return Theme.of(context).colorScheme.primary;
   }
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 2e6bfc1 and eebdf97.

📒 Files selected for processing (16)
  • lib/core/utils/date_utils.dart (1 hunks)
  • lib/features/orders/data/datasources/order_local_data_source.dart (1 hunks)
  • lib/features/orders/data/datasources/order_remote_data_source.dart (1 hunks)
  • lib/features/orders/data/models/order_model.dart (1 hunks)
  • lib/features/orders/data/repositories/order_repository_impl.dart (1 hunks)
  • lib/features/orders/domain/entities/order.dart (1 hunks)
  • lib/features/orders/domain/repositories/order_repository.dart (1 hunks)
  • lib/features/orders/domain/usecases/get_orders.dart (1 hunks)
  • lib/features/orders/domain/usecases/update_order_status.dart (1 hunks)
  • lib/features/orders/presentation/bloc/order_bloc.dart (1 hunks)
  • lib/features/orders/presentation/bloc/order_event.dart (1 hunks)
  • lib/features/orders/presentation/bloc/order_state.dart (1 hunks)
  • lib/features/orders/presentation/screens/order_details_screen.dart (1 hunks)
  • lib/features/orders/presentation/screens/order_list_screen.dart (3 hunks)
  • lib/features/orders/presentation/widgets/order_item.dart (1 hunks)
  • lib/injection_container.dart (2 hunks)
🔇 Additional comments (11)
lib/features/orders/data/datasources/order_local_data_source.dart (2)

167-169: Handle date-time conversion carefully

When updating completedAt, ensure that the date-time is stored in a consistent format and time zone.

Confirm that DateTime.now().toIso8601String() stores the date in the correct format. If necessary, consider using UTC.


77-79: Ensure completedAt is always stored correctly

When inserting into the database, make sure that completedAt is handled consistently, even when it's null.

Run the following script to ensure that completedAt field is stored correctly:

✅ Verification successful

completedAt field is handled correctly in the codebase

The code handles the completedAt field appropriately:

  • In the database schema, it's defined as nullable (TEXT)
  • When inserting orders, it's only included if the value is non-null
  • When updating order status to completed, it's properly set:
    • In remote datasource: uses server timestamp
    • In local datasource: uses current timestamp
  • When parsing orders, it correctly handles both null and non-null cases
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all orders have 'completedAt' set appropriately.

# List orders without 'completedAt' where status is 'completed'
sqlite3 orders.db "SELECT id FROM orders WHERE status = 'completed' AND completedAt IS NULL;"

Length of output: 199


Script:

#!/bin/bash
# Let's analyze how completedAt is handled in the codebase

# Find all references to completedAt field
rg "completedAt" -A 3 -B 3

# Also check for any database schema definitions
fd "schema|model" --type f

Length of output: 7322

lib/features/orders/domain/repositories/order_repository.dart (1)

6-12: LGTM! Well-structured repository interface.

The repository pattern is well implemented with proper error handling using Either type.

lib/features/orders/presentation/bloc/order_event.dart (1)

13-24: LGTM! Well-structured event class.

The UpdateOrder event is well-implemented with proper immutability, required parameters, and correct Equatable implementation.

lib/core/utils/date_utils.dart (1)

17-18: LGTM! More readable date format.

The change from 'MM/dd' to 'd MMM' improves readability by using abbreviated month names instead of numbers.

lib/features/orders/presentation/bloc/order_state.dart (2)

5-6: LGTM! Good use of const constructor.

Adding const constructor to the base class enables compile-time constants for all state classes.


16-21: LGTM! Proper immutable state implementation.

Good implementation of the OrdersLoaded state with:

  • Const constructor
  • Final field
  • Proper props override
lib/features/orders/domain/usecases/update_order_status.dart (2)

7-15: LGTM! Clean implementation of the use case.

Good implementation following clean architecture principles:

  • Proper dependency injection
  • Error handling with Either
  • Single responsibility

12-14: Verify error handling in the repository implementation.

The use case correctly delegates to the repository, but let's verify the error handling implementation.

✅ Verification successful

Error handling in the repository implementation is properly implemented

The repository implementation has comprehensive error handling that covers:

  • Network connectivity checks via networkInfo.isConnected
  • Authentication errors through AuthException handling
  • Network failures with appropriate NetworkFailure responses
  • Both remote and local data source updates with proper error propagation
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for the repository implementation to verify error handling
ast-grep --pattern 'class $_ implements OrderRepository {
  $$$
  Future<Either<Failure, void>> updateOrderStatus($_, $_) {
    $$$
  }
  $$$
}'

Length of output: 146


Script:

#!/bin/bash
# Let's try a broader search for OrderRepository implementations
rg "implements OrderRepository" -A 10

# Also search for the updateOrderStatus method
rg "updateOrderStatus.*Failure.*void"

Length of output: 1116


Script:

#!/bin/bash
# Let's look at the complete implementation file to see the updateOrderStatus method
cat lib/features/orders/data/repositories/order_repository_impl.dart

Length of output: 2851

lib/features/orders/presentation/screens/order_details_screen.dart (1)

1-14: LGTM! Clean class structure and imports.

The class follows Flutter best practices with proper const constructor and required parameter.

lib/injection_container.dart (1)

30-35: LGTM! Well-organized imports for order feature.

All necessary components for the order feature are properly imported.

Comment on lines +45 to +47
Logger().e('Error fetching orders: $e');
throw AuthException('Failed to fetch orders: $e');
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid exposing sensitive error details in exceptions

Including the original exception message $e in the thrown AuthException might expose sensitive information. It's better to provide a generic error message to prevent potential information leakage.

Apply this diff to modify the exception message:

- throw AuthException('Failed to fetch orders: $e');
+ throw AuthException('Failed to fetch orders');
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Logger().e('Error fetching orders: $e');
throw AuthException('Failed to fetch orders: $e');
}
Logger().e('Error fetching orders: $e');
throw AuthException('Failed to fetch orders');
}

)
.toList();
} catch (e) {
Logger().e('Error fetching orders: $e');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid logging sensitive error information

Logging the exception $e can reveal sensitive details in the logs. Consider logging only the error type or a generic message.

Apply this diff to adjust the logging:

- Logger().e('Error fetching orders: $e');
+ Logger().e('Error fetching orders');
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Logger().e('Error fetching orders: $e');
Logger().e('Error fetching orders');

Comment on lines +62 to +64
Logger().e('Error updating order status: $e');
throw AuthException('Failed to update order status: $e');
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consistent error handling and prevent sensitive data exposure

Similar to the getOrders method, include error handling in updateOrderStatus and avoid exposing sensitive error details in both logging and exceptions.

Apply this diff to add error handling and prevent information leakage:

} catch (e) {
-   Logger().e('Error updating order status: $e');
-   throw AuthException('Failed to update order status: $e');
+   Logger().e('Error updating order status');
+   throw AuthException('Failed to update order status');
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Logger().e('Error updating order status: $e');
throw AuthException('Failed to update order status: $e');
}
Logger().e('Error updating order status');
throw AuthException('Failed to update order status');
}

Comment on lines +68 to +70
Future<void> createOrder(OrderModel order) async {
await _firestore.collection('orders').doc().set(order.toMap());
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling to createOrder method

The createOrder method lacks error handling, which can lead to unhandled exceptions. Wrap the method's body in a try-catch block to handle potential errors and maintain consistency with other methods.

Apply this diff to include error handling:

@override
Future<void> createOrder(OrderModel order) async {
+ try {
    await _firestore.collection('orders').doc().set(order.toMap());
+ } catch (e) {
+   Logger().e('Error creating order');
+   throw AuthException('Failed to create order');
+ }
}

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +62 to +63
Logger().e('Failed to update order: ${failure.message}');
emit(OrdersError(failure.message));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid logging sensitive error details

As with the _onLoadOrders method, avoid logging the failure message directly.

Apply this diff to adjust the logging:

- Logger().e('Failed to update order: ${failure.message}');
+ Logger().e('Failed to update order');
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Logger().e('Failed to update order: ${failure.message}');
emit(OrdersError(failure.message));
Logger().e('Failed to update order');
emit(OrdersError(failure.message));

Comment on lines +17 to +37
factory OrderModel.fromMap(Map<String, dynamic> map, String id) {
return OrderModel(
id: id,
businessId: map['businessId'] as String,
businessName: map['businessName'] as String,
items: (map['items'] as List<dynamic>)
.map(
(dynamic item) =>
OrderItemModel.fromMap(item as Map<String, dynamic>),
)
.toList(),
totalAmount: (map['totalAmount'] as num).toDouble(),
status: order_entity.OrderStatus.values.firstWhere(
(order_entity.OrderStatus s) => s.name == (map['status'] as String),
),
createdAt: (map['createdAt'] as Timestamp).toDate(),
completedAt: map['completedAt'] != null
? (map['completedAt'] as Timestamp).toDate()
: null,
);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling to fromMap factory

The fromMap factory should handle potential null or invalid values from Firestore.

factory OrderModel.fromMap(Map<String, dynamic> map, String id) {
  try {
    return OrderModel(
      id: id,
      businessId: map['businessId'] as String? ?? 
          throw FormatException('businessId is required'),
      // ... similar null checks for other fields
    );
  } catch (e) {
    throw FormatException('Failed to parse OrderModel: $e');
  }
}

Comment on lines +39 to +53
Map<String, dynamic> toMap() {
return <String, dynamic>{
'businessId': businessId,
'businessName': businessName,
'items': items
.map(
(order_entity.OrderItem item) => (item as OrderItemModel).toMap(),
)
.toList(),
'totalAmount': totalAmount,
'status': status.name,
'createdAt': Timestamp.fromDate(createdAt),
if (completedAt != null) 'completedAt': Timestamp.fromDate(completedAt!),
};
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add type safety to toMap serialization

The type casting in the items map operation could fail at runtime.

Map<String, dynamic> toMap() {
  if (items.any((item) => item is! OrderItemModel)) {
    throw StateError(
      'All items must be OrderItemModel instances for serialization',
    );
  }
  return <String, dynamic>{
    'businessId': businessId,
    // ... rest of the implementation
  };
}

Comment on lines +60 to +79
...order.items.map(
(order_entity.OrderItem item) => Padding(
padding: const EdgeInsets.only(bottom: 12.0),
child: Row(
mainAxisAlignment: MainAxisAlignment.spaceBetween,
children: <Widget>[
Expanded(
child: Text(
'${item.quantity}x ${item.name}',
style: theme.textTheme.bodyLarge,
),
),
Text(
'COP ${item.price.toStringAsFixed(2)}',
style: theme.textTheme.bodyLarge,
),
],
),
),
),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve currency formatting and text handling.

Consider these improvements:

  1. Use proper currency formatting instead of hardcoded 'COP'
  2. Handle potential text overflow for long item names
 ...order.items.map(
   (order_entity.OrderItem item) => Padding(
     padding: const EdgeInsets.only(bottom: 12.0),
     child: Row(
       mainAxisAlignment: MainAxisAlignment.spaceBetween,
       children: <Widget>[
         Expanded(
           child: Text(
             '${item.quantity}x ${item.name}',
+            overflow: TextOverflow.ellipsis,
             style: theme.textTheme.bodyLarge,
           ),
         ),
         Text(
-          'COP ${item.price.toStringAsFixed(2)}',
+          NumberFormat.currency(
+            symbol: 'COP ',
+            decimalDigits: 2,
+          ).format(item.price),
           style: theme.textTheme.bodyLarge,
         ),
       ],
     ),
   ),
 ),

Don't forget to add the import:

import 'package:intl/intl.dart';

Comment on lines +82 to +97
// Total
Row(
mainAxisAlignment: MainAxisAlignment.spaceBetween,
children: <Widget>[
Text(
'Total',
style: theme.textTheme.titleLarge,
),
Text(
'COP ${order.totalAmount.toStringAsFixed(2)}',
style: theme.textTheme.titleLarge?.copyWith(
fontWeight: FontWeight.bold,
),
),
],
),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use consistent currency formatting for total amount.

Apply the same currency formatting improvement as suggested for the order items.

 Text(
-  'COP ${order.totalAmount.toStringAsFixed(2)}',
+  NumberFormat.currency(
+    symbol: 'COP ',
+    decimalDigits: 2,
+  ).format(order.totalAmount),
   style: theme.textTheme.titleLarge?.copyWith(
     fontWeight: FontWeight.bold,
   ),
 ),

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +186 to +216
serviceLocator.registerFactory(
() => OrderBloc(
getOrders: serviceLocator(),
updateOrderStatus: serviceLocator(),
),
);

// Use cases
serviceLocator.registerLazySingleton(() => GetOrders(serviceLocator()));
serviceLocator
.registerLazySingleton(() => UpdateOrderStatus(serviceLocator()));

// Repository
serviceLocator.registerLazySingleton<OrderRepository>(
() => OrderRepositoryImpl(
remoteDataSource: serviceLocator(),
localDataSource: serviceLocator(),
networkInfo: serviceLocator(),
),
);

// Data sources
serviceLocator.registerLazySingleton<OrderRemoteDataSource>(
() => OrderRemoteDataSourceImpl(
firestore: serviceLocator(),
auth: serviceLocator(),
),
);
serviceLocator.registerLazySingleton<OrderLocalDataSource>(
() => OrderLocalDataSourceImpl(),
);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding network error handling in OrderBloc.

While the dependency injection setup is good, consider injecting InternetConnectionBloc for proper network error handling, similar to ProfileBloc implementation.

 serviceLocator.registerFactory(
   () => OrderBloc(
     getOrders: serviceLocator(),
     updateOrderStatus: serviceLocator(),
+    internetConnectionBloc: serviceLocator(),
   ),
 );

Committable suggestion skipped: line range outside the PR's diff.

@abelarismendy abelarismendy merged commit ac1c5ed into main Dec 2, 2024
1 check passed
@abelarismendy abelarismendy mentioned this pull request Dec 2, 2024
@coderabbitai coderabbitai bot mentioned this pull request Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants